-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Use Spring's native CSRF protection, and fix login page #37292
base: release
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduced in this pull request enhance the security and functionality of the application by adding CSRF protection and improving API request handling. Key modifications include the addition of headers in API commands, the introduction of a CSRF token input component in user authentication forms, and the implementation of a CSRF configuration class in the server-side code. Error handling and validation checks have also been improved across various components, ensuring a more robust and secure application. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
app/client/src/pages/UserAuth/CsrfTokenInput.tsx (1)
4-5
: Consider using a more robust cookie parsing approach.While the regex works, consider using a dedicated cookie parsing utility for better maintainability and reliability.
-const csrfToken: string = - document.cookie.match(/\bXSRF-TOKEN=([-a-z0-9]+)/i)?.[1] ?? ""; +import Cookies from 'js-cookie'; + +const csrfToken: string = Cookies.get('XSRF-TOKEN') ?? "";app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java (3)
26-31
: Consider adding security headers verification.While the setup is good, consider extending it to verify security headers in responses (X-Frame-Options, X-Content-Type-Options, etc.) as part of CSRF protection testing.
@BeforeEach void setup(ReactiveWebApplicationContext context) { webTestClient = WebTestClient.bindToApplicationContext(context) .apply(springSecurity()) + .configureClient() + .defaultHeaders(headers -> headers.add("X-Requested-By", "Appsmith")) .build(); }
33-66
: Add documentation for test scenarios.The test method effectively covers various CSRF scenarios, but would benefit from explicit documentation of test cases.
@ParameterizedTest @MethodSource("testParams") +/** + * Tests CSRF protection with following scenarios: + * 1. Valid CSRF token in both form and cookie + * 2. Invalid CSRF token in form + * 3. Missing CSRF token in cookie + * 4. Mismatched tokens + */ void testCsrf(TestParams t) {
36-36
: Consider using SecureRandom for token generation.Using UUID for CSRF tokens is acceptable but consider using
SecureRandom
for stronger token generation in security-critical tests.-final String tokenValue = UUID.randomUUID().toString(); +final String tokenValue = new BigInteger(130, new SecureRandom()).toString(32);app/client/src/pages/setup/DetailsForm.tsx (1)
109-109
: Consider adding error handling for missing CSRF token.The CsrfTokenInput is rendered unconditionally. Consider adding error handling in case the CSRF token cookie is not present.
- <CsrfTokenInput /> + <ErrorBoundary fallback={<div>Failed to load CSRF token. Please refresh the page.</div>}> + <CsrfTokenInput /> + </ErrorBoundary>app/client/src/pages/UserAuth/Login.tsx (1)
189-189
: Consider enhancing form securityWhile the CSRF protection is a good addition, consider these security enhancements:
- Add rate limiting for failed login attempts
- Implement password strength validation
- Add client-side email validation before form submission
Also applies to: 190-190, 191-191, 192-192, 193-193, 194-194, 195-195, 196-196, 197-197, 198-198, 199-199, 200-200, 201-201, 202-202, 203-203, 204-204, 205-205, 206-206, 207-207, 208-208, 209-209, 210-210, 211-211, 212-212, 213-213, 214-214, 215-215, 216-216, 217-217, 218-218, 219-219, 220-220, 221-221, 222-222, 223-223, 224-224, 225-225, 226-226, 227-227, 228-228, 229-229
app/client/src/pages/UserAuth/SignUp.tsx (1)
247-247
: Consider adding error handling for CSRF token validation failuresWhile the CsrfTokenInput is correctly placed, consider adding error handling for cases where CSRF token validation fails. This would improve the user experience by showing appropriate error messages.
<SpacedSubmitForm action={signupURL.toString()} id="signup-form" method="POST" onSubmit={(e) => handleSubmit(e)} > - <CsrfTokenInput /> + <CsrfTokenInput + onError={(error) => { + // Handle CSRF token validation error + showError = true; + errorMessage = "Session expired. Please refresh the page."; + }} + />app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (1)
Line range hint
169-270
: Architecture review: Security configuration maintains robust protectionThe security configuration:
- Preserves essential security features (rate limiting, authentication chains)
- Maintains secure cookie configuration
- Successfully integrates Spring's CSRF protection
Consider documenting the CSRF token requirements in the API documentation for client implementations.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java (1)
81-83
: Clarify the comment for accurate contextThe comment may be misleading. Since
SAFE_READ_ONLY_METHODS
includesGET
andHEAD
, which are methods supported by HTML forms, consider updating the comment to reflect that CSRF checks are not required for safe, read-only methods.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
app/client/cypress/support/commands.js
(2 hunks)app/client/src/pages/UserAuth/CsrfTokenInput.tsx
(1 hunks)app/client/src/pages/UserAuth/Login.tsx
(2 hunks)app/client/src/pages/UserAuth/SignUp.tsx
(2 hunks)app/client/src/pages/setup/DetailsForm.tsx
(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java
(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java
(0 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java
(1 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/commands.js (1)
Pattern app/client/cypress/**/**.*
: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (10)
app/client/src/pages/UserAuth/CsrfTokenInput.tsx (1)
1-8
: LGTM! The component correctly implements CSRF token handling.
The component successfully extracts and provides the CSRF token in the expected format for Spring Security.
Let's verify the CSRF token usage in forms:
app/client/src/pages/setup/DetailsForm.tsx (2)
27-27
: LGTM: Clean import of CsrfTokenInput component.
109-109
: Verify CSRF token handling in form submission.
The CsrfTokenInput is correctly placed at the start of the form. However, let's verify that the form submission handler properly includes this token.
✅ Verification successful
CSRF token handling is properly implemented
The implementation is secure because:
- The
CsrfTokenInput
component correctly extracts the CSRF token from cookies - The global axios instance in
Api.ts
is configured withwithCredentials: true
- The request interceptor adds
X-Requested-By: Appsmith
header - All API requests are automatically handled through this configured axios instance
The form submission will include the CSRF token as it's properly set up as a hidden form input, and the API layer handles the rest of the security measures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for form submission handling code
rg -t typescript -t javascript "handleSubmit|onSubmit.*DetailsForm"
# Look for any existing CSRF token validation
ast-grep --pattern 'fetch($_, {
$$$
headers: {
$$$
"X-CSRF-Token": $_,
$$$
}
$$$
})'
Length of output: 211
Script:
#!/bin/bash
# Search for form submission handling code with correct file extension
rg -t ts -t tsx -t js -t jsx "handleSubmit|onSubmit.*DetailsForm"
# Look for CSRF token related code in the DetailsForm
rg -t ts -t tsx -t js -t jsx "CsrfTokenInput|csrf|CSRF" "app/client/src/pages/setup/DetailsForm.tsx" -A 5 -B 5
# Look for axios or fetch calls in DetailsForm
rg -t ts -t tsx "axios\.|fetch\(" "app/client/src/pages/setup/DetailsForm.tsx" -A 5
# Check CsrfTokenInput implementation
fd "CsrfTokenInput" --type f --exec cat {}
Length of output: 626
Script:
#!/bin/bash
# Search for form submission handling with ripgrep using file content matching
rg -g "*.{ts,tsx,js,jsx}" "handleSubmit|onSubmit.*DetailsForm"
# Look for form submission or API calls in DetailsForm using file content matching
rg -g "*.{ts,tsx,js,jsx}" "axios|fetch|submit" "app/client/src/pages/setup/DetailsForm.tsx"
# Look for the form element and its props in DetailsForm
rg -g "*.{ts,tsx,js,jsx}" "<form.*>|onSubmit={" "app/client/src/pages/setup/DetailsForm.tsx" -A 5 -B 5
Length of output: 3839
Script:
#!/bin/bash
# Look for form element and its props in DetailsForm with fixed regex
rg -g "*.{ts,tsx,js,jsx}" "<form|onSubmit" "app/client/src/pages/setup/DetailsForm.tsx" -A 5 -B 5
# Look for any HTTP client setup or configuration files
fd -e ts -e tsx -e js "api|http|client|axios" --type f --exec rg -l "csrf|CSRF"
# Check if there's any global HTTP client configuration
rg -g "*.{ts,tsx,js,jsx}" "axios\.defaults|fetch.*headers"
Length of output: 2204
Script:
#!/bin/bash
# Check the API request interceptors implementation
cat app/client/src/api/__tests__/apiRequestInterceptors.ts
# Check the main API configuration
cat app/client/src/api/Api.ts
# Look for form submission in DetailsForm with broader context
rg -g "*.{ts,tsx}" "function.*DetailsForm|const.*DetailsForm" app/client/src/pages/setup/DetailsForm.tsx -A 20
Length of output: 9102
app/client/src/pages/UserAuth/Login.tsx (2)
56-57
: LGTM: Clean import of CSRF component
The import is appropriately placed with other component imports.
189-189
: Verify CSRF token handling in form submission
The CSRF token input is correctly placed at the start of the form. However, we should verify that:
- The form's POST action to
/api/v1/login
properly includes the CSRF token - The Spring backend is configured to validate this token
✅ Verification successful
CSRF protection is properly implemented
The implementation is secure with:
- Frontend:
CsrfTokenInput
component correctly extracts token from cookie and adds it as hidden form input - Backend: Comprehensive CSRF configuration in
CsrfConfig.java
with:- Token validation for form submissions
- Proper cookie handling with
CookieServerCsrfTokenRepository
- Security exceptions for safe methods (GET) and API requests
- Token regeneration on logout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the login endpoint is properly configured in Spring Security
rg -g '*.java' -A 5 'configure.*HttpSecurity.*login'
# Verify CSRF configuration
rg -g '*.java' -A 5 'csrf\(\).*disable|csrf\(\).*enable'
Length of output: 113
Script:
#!/bin/bash
# Let's check the CsrfTokenInput component implementation
fd -e tsx -e ts "CsrfTokenInput" --exec cat {}
# Check form submission handling in Login component
rg -g "Login.tsx" "onSubmit|handleSubmit" -A 5
# Look for CSRF configuration in security config files
fd -e java "Security" --exec cat {}
# Check if there are any CSRF related utilities or services
rg -g "*.{ts,tsx,java}" "csrf" -i
Length of output: 29683
app/client/src/pages/UserAuth/SignUp.tsx (2)
62-62
: LGTM: Clean import addition
The CsrfTokenInput import is properly organized with other component imports.
247-247
: Verify CSRF token handling in form submission
The CsrfTokenInput is correctly placed at the start of the form. However, since we're using direct form submission via formElement.submit()
, let's verify that the CSRF token is properly included in the request.
✅ Verification successful
CSRF token implementation is correctly handled
The implementation is secure:
- CsrfTokenInput correctly extracts the XSRF token from cookies
- Token is properly included as a hidden form field named "_csrf"
- API interceptors validate CSRF with X-Requested-By header
- Direct form submission will include the CSRF token automatically
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any CSRF-related tests in the codebase
rg -g '*.{spec,test}.{ts,tsx,js,jsx}' -l 'csrf|CSRF'
# Check if there are any CSRF-related Cypress commands or tests
rg -g 'cypress/**/*.{js,ts}' -l 'X-Requested-By.*Appsmith'
Length of output: 264
Script:
#!/bin/bash
# Check CSRF token implementation and usage
rg -g '*.{ts,tsx,js,jsx}' 'CsrfTokenInput'
# Check form submission handling in SignUp.tsx
rg -g 'SignUp.tsx' -A 5 'formElement.submit'
# Check if there's any CSRF middleware or configuration
rg -g '*.{ts,js}' 'csrf|CSRF' --type-add 'config:*.config.*' --type config
Length of output: 1654
Script:
#!/bin/bash
# Check CsrfTokenInput implementation
cat app/client/src/pages/UserAuth/CsrfTokenInput.tsx
# Check API interceptors for CSRF handling
cat app/client/src/api/__tests__/apiRequestInterceptors.ts
# Check form element attributes in SignUp.tsx
rg -g 'SignUp.tsx' '<form.*>' -A 2
Length of output: 4262
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (2)
114-116
: LGTM: Clean dependency injection of CsrfConfig
The autowiring of CsrfConfig aligns with Spring's dependency injection patterns and the PR's objective to use Spring's native CSRF protection.
169-170
: Verify CSRF token handling in authentication flows
The transition to Spring's native CSRF protection is good, but we should ensure proper CSRF token handling in:
- Login form submissions
- OAuth2 authentication flows
- API endpoints that were previously CSRF-exempt
Let's verify the CSRF configuration:
✅ Verification successful
CSRF protection is properly configured for authentication flows
The verification shows:
- Spring's CSRF protection is correctly applied via
CsrfConfig
before any security filters - OAuth2 endpoints (
/oauth2/**
) are properly CSRF-exempt viapermitAll
- Login endpoint is protected with rate limiting and proper authentication handling
- No legacy CSRF filters remain in the codebase, confirming complete migration to Spring's native CSRF protection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for CSRF token handling in authentication-related files
echo "Checking CSRF token handling in authentication flows..."
rg -l "csrf|CSRF" --type java --glob "**/*Authentication*.java"
# Look for any remaining custom CSRF implementations
echo "Checking for any remaining custom CSRF implementations..."
rg -l "CSRFFilter|CsrfFilter" --type java
Length of output: 355
Script:
#!/bin/bash
# Let's check SecurityConfig for CSRF configuration
echo "Checking SecurityConfig for CSRF configuration..."
rg -A 10 -B 10 "csrf" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
# Check for OAuth2 related configurations
echo "Checking OAuth2 configurations..."
rg -l "oauth2|OAuth2" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/
# Check for authentication endpoints and their security configuration
echo "Checking authentication endpoints..."
rg -A 5 "antMatchers.*auth|permitAll|authenticated" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
# Check for any custom security filters
echo "Checking custom security filters..."
fd -e java --exec rg -l "extends.*Filter|implements.*Filter" {} \;
Length of output: 6085
app/client/cypress/support/commands.js (1)
943-943
: LGTM: Added CSRF protection header.
The addition of the X-Requested-By
header in the signup API call is consistent with the CSRF protection implementation.
app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java
Outdated
Show resolved
Hide resolved
Failed server tests
|
Description
The login page, and a few other pages are exempted from CSRF today and aren't doing any check at all. This makes our login page vulnerable to CSRF. But it's not really exploitable in its current form, since there's other points in the login flow that patch this hole up.
Nevertheless, CSRF being possible on the login form doesn't sound good in any tone and context. This PR fixes this by not exempting anything from CSRF, and doing a stateless CSRF check where necessary.
PR summary:
X-Requested-By: Appsmith
for the login/signup form submission calls from Cypress.Content-Type: application/json
header. Previously, if a request had this header, it was considered exempt from CSRF check. This has been removed as it appears it's not a safe assumption in today's JSON-dominated web.Automation
/test all
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11840865597
Commit: 05db7b6
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
Thu, 14 Nov 2024 17:21:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
CsrfTokenInput
component to enhance security during user authentication and signup processes by including a CSRF token.Improvements
Bug Fixes
Tests